Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add workspace initialization command #374

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

AlemTuzlak
Copy link

Summary

Added a command that allows you to initalize biome settings in the workspace based on this tweet:
https://x.com/colinhacks/status/1841219884287737886
and this dicussion:
#230

Description

when you right click on either setttings.json in .vscode or biome.json you can click on the biome: initalize workspace command to add all the required fields into settings.json for biome to work properly, also check this tweet for the merging of the config files:
https://x.com/andhaveaniceday/status/1841226098706956625

Feel free to edit and add whatever you deem is needed here

Checklist

  • I have tested my changes on the following platforms:
    • Windows
    • Linux
    • macOS

@AlemTuzlak AlemTuzlak requested review from nhedger and a team as code owners October 2, 2024 13:52
Comment on lines +57 to +62
"[javascript]",
"[typescript]",
"[typescriptreact]",
"[javascriptreact]",
"[json]",
"[jsonc]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add vue, astro, svelte, css and graphql

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added it in as well

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico If this means Biome will take precedence over the built-in extensions for vue/astro/svelte files, I'm not sure that's a great idea until support has progressed further and stabilized. I suspect anyone using these frameworks in earnest will prefer the official extensions at the moment, which means we'll need to document how to remove these lines from settings.json/.code-workspace (which is the kind of complexity that tends to intimidate new users).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a valid point, I left it generic in the beginning as I wasn't sure if I should add specific languages as well but yeah this is definitely something to think about from the maintainers side of view!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @colinhacks makes a good point here. We should only include languages for which we have full formatting support.

This would mean removing vue, svelte, and astro for now.

@nhedger nhedger changed the title feature: Add initialize project command feat: add workspace initialization command Oct 2, 2024
@nhedger
Copy link
Member

nhedger commented Oct 2, 2024

Thanks for the contrib.

How does that behave when in a a multi-root workspace ?

@AlemTuzlak
Copy link
Author

@nhedger seems to get added to test.code-workspace instead of settings.json in the individual ones, what would be the right behaviour here?

@nhedger
Copy link
Member

nhedger commented Oct 2, 2024

@nhedger seems to get added to test.code-workspace instead of settings.json in the individual ones, what would be the right behaviour here?

I think we'd want the config to be added to the .vscode/settings.json file if the action was triggered from the context menu on that file. Maybe we could also add a context menu for the .code-workspace file, WDYT?

@AlemTuzlak
Copy link
Author

@nhedger when you click on the .code-workspace it will generate it on the root of that workspace into a separate .vscode folder, and if you click on the workspaces inside it will generate it there, would that be the expected behavior?

@nhedger
Copy link
Member

nhedger commented Oct 2, 2024

I would expect it to add the config inside the .code-workspace file in the settings section.

@AlemTuzlak
Copy link
Author

@nhedger I can't get it to output it to the .code-workspace file at all, seems that vscode isn't happy with that, when i scope it to the .code-workspace file it's still output to the .vscode folder and whatever I do it's the same result, the only way to allow it on .code-workspace is to manually parse the file and rewrite it with fs it seems, unless I'm missing something.

@AlemTuzlak
Copy link
Author

my findings:
workspace1/.vscode/settings => if you set output to Workspace it's output to the .code-workspace
workspace1/.vscode/settings => if you set output to WorkspaceFolder AND scope it to the current path its output properly (what this PR does)
.code-workspace => no scope / scoped to workspace path with Workspace is output to .vscode folder neighbouring it.
.code-workspace => no scope / scoped to workspace path with WorkspaceFolder nothing happens
.code-workspace => no scope / scoped to workspace path with Global it's output to global settings above the workspace

@AlemTuzlak
Copy link
Author

you can easily check if it's a .code-workspace by checking if the args.path ends with that extension and feel free to try it out yourself and see if i've missed something obvious. From my perspective the PR is ready as is, but that feature can be added on top of course!

package.json Outdated Show resolved Hide resolved
@colinhacks
Copy link

colinhacks commented Oct 2, 2024

Thanks Alem, I love this!

I do have a question for the maintainers. Why are the per-language settings necessary in the first place? If Biome is set as the top-level default formatter in a workspace, it seems surprising that is isn't used by default for all file types that Biome is capable of formatting. I'm sure this was an intentional decision, but I'd love to see it revisited, especially as Biome matures (and with a new major of the extension on the horizon).

If Biome adds support for additional file types down the line, those will need to be added. This is likely to cause confusion for users who have run the "Biome: Initialize" and assumed Biome will just work moving forward. (On the flip side of this coin, it's probably not a good idea to override the official Svelte/Vue/Astro extensions yet, see my comment in the review interface.)

@ematipico
Copy link
Member

I do have a question for the maintainers. Why are the per-language settings necessary in the first place? If Biome is set as the top-level default formatter in a workspace, it seems surprising that is isn't used by default for all file types that Biome is capable of formattin

It isn't necessary, actually. For example, I use the following setting on my VSCode:

{
	"editor.defaultFormatter": "biomejs.biome",
	"editor.formatOnSave": true,
}

These settings work as expected. If they don't, there's probably a bug lurking somewhere (either the extension or the editor).

I suppose the docs are outdated, and at that time, we suggested an opt-in approach not to disrupt the user's settings and default formatter. We are happy to revisit them if you think it's more valuable. There's an open PR for the upcoming v3 of the extension if you're interested: biomejs/website#974

@AlemTuzlak
Copy link
Author

@ematipico I also believe there is a bug somewhere, we have been extensively using biome in our company and we had to play around with the settings.json a lot to get it working properly with biome.

I've added the suggested changes by @colinhacks to the PR

@AlemTuzlak
Copy link
Author

Hey guys, any updates on this, anything on my side I need to do? Really interested if it will be merged or not!

@nhedger
Copy link
Member

nhedger commented Nov 12, 2024

Hey @AlemTuzlak, haven't had a chance to test it out yet. I've been taking a break. I'll try to review and test it this week.

@AlemTuzlak
Copy link
Author

@nhedger no worries, just wanted to confirm I'm not the cause of it. Great to hear, take your time

Copy link
Member

@nhedger nhedger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I took some time to go through the PR.

Seems to work well overall!

Biome: Initialize command

Calling the Biome: Initialize command from the command palette produces an error message. I suspect it's because the code expects a path that isn't provided when calling the command this way.

I think we should either hide the command from the command palette, or make sure that it works when called this way. I haven't looked into it deeply, not sure if/how it's doable.

FYI, I reached for the command through the palette when testing workspaces in which there was no Biome configuration file, nor VS Code settings, so I could not access the context menu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants